-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cli: hint for conflicted branches and change ids #2244
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hi! I've done the above but I'm still not too sure about when |
Thanks for your contribution, the CLA will be valid after the next push. |
3bfeb7f
to
310e72a
Compare
I tried to address the comments and also added another separate hint for when there's commits with the same change id. Not too sure about tests yet since I don't know how to easily reproduce these situations. Also thanks for the review! |
Re PR description: it might be more accurate to say that this resolves a portion of #2192. There are also the other kinds of conflicts. Update: I missed where you said that you added something about change ids. You should split that into a separate commit. |
For the tests, you could base a test on one of the Alternatively, you can also try adding a test there that does the equivalent of: let pre_creation_id = test_env.current_operation_id(&repo_path);
test_env.jj_cmd_success(&repo_path, &["branch", "create", "a", "-r=one_commit"]);
test_env.jj_cmd_success(&repo_path, &["branch", "create", "a", "-r=another_commit",
"--at-op", pre_creation_id]); This could go into https://github.com/martinvonz/jj/blob/main/cli/tests/test_branch_command.rs. There aren't many tests with conflicted branches there at the moment, but they might be nice to have. By the way, the tests should go either into the same commit where you make the change or in a commit before that, so that the commit where you make the change also changes the test. |
7b02752
to
41b9190
Compare
Hi! I've added the tests in (The snippet didn't create a branch conflict for me when testing locally) let pre_creation_id = test_env.current_operation_id(&repo_path);
test_env.jj_cmd_success(&repo_path, &["branch", "create", "a", "-r=one_commit"]);
test_env.jj_cmd_success(&repo_path, &["branch", "create", "a", "-r=another_commit",
"--at-op", pre_creation_id]); |
b7aa67c
to
eb27a38
Compare
Hi, I believe I addressed all the requested changes! (Not sure if this is sufficient for the issue) |
Thanks! I had started another round of reviewing and then got distracted. I'll finish that now |
Hi, I just made the changes. Thanks for the feedback on the tests, it ended up being way simpler! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Extracting that commits_summary
variable was a bigger improvement that I had expected.
e217e15
to
b2ccc89
Compare
Resolves issue #2192
The output of the changed hint is:
Checklist
If applicable:
CHANGELOG.md